-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement OpenVox support #165
base: master
Are you sure you want to change the base?
Conversation
I know that this isn't ready yet. The focus is on creating beaker jobs for puppet and openvox packages. unit tests will continue with the |
f0bf384
to
151acff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is what I'd do. In the the what I'd like is a single variable (both for unit and acceptance, so really 2 vars) that generates the entire matrix. That can be both puppet and openvox. We exposed 1 variable that gha uses now. Compatible solution is to just expand that result, even if the name includes puppet. Incompatible is to have genetic names like puppet_metadata_unit_matrix
or similar. Prefix it with the gem name and a suffix for the purpose.
Looking further ahead we probably also want to do similar things with unit tests to test with agent gems
Yes. But since we don't have the gem published at the moment, it didn't have a high priority for me.
That's my idea. I want to expand it, if |
Right, so let's start 2 variable that are generic: On the implementation details, for acceptance I'd say it osrequirementversions. So centos 9 * {puppet,openvox} * 8 for example. It may be more complex, but that's up to this gem to decide. Similar for unit tests |
# Current latest major is 7. It is highly recommended that modules | ||
# actually specify exact bounds, but this prevents an infinite loop. | ||
end_major = (requirement.end == SemanticPuppet::Version::MAX) ? 7 : requirement.end.major |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks out of date, which is a problem with hardcoding magic numbers ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not worth the dependency, but we could depend on rspec-puppet-facts, which ships https://github.com/voxpupuli/rspec-puppet-facts/blob/master/ext/puppet_agent_components.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really not mind to see hard failures when an upper bound is not set.
6d72ccd
to
1f335e8
Compare
15aaaf2
to
ae38ddf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of these things are me thinking out loud. I'd welcome counter arguments.
# when we want to make this dynamic, switch to: | ||
# "BEAKER_#{requirement_name.upcase}_COLLECTION" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out loud: today we use beaker_puppet_helpers and this variable configures that. If we look at the values it supports then there are things like puppet7
, puppet8
and with https://github.com/voxpupuli/beaker_puppet_helpers/pull/56
also openvox8
. I think looking at multiple env vars is going to be a lot more messy so I'm not sure I'd like it.
Perhaps beaker_puppet_helpers
deserves a new name and once that's there we can pick a new env var. Or there are other solutions, but iterating env vars for supported values is something I'd like to avoid. Since it's an environment we control I'd even prefer an env var to indicate which requirement to look at.
One thing where we may find an issue is the distro version, but I doubt distros are going to package both Puppet and OpenVox.
# @return [Array[Integer]] Supported major Puppet versions | ||
# @see #requirements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked having methods documented with yardoc tags.
@@ -48,15 +60,19 @@ def puppet_unit_test_matrix | |||
end.compact | |||
end | |||
|
|||
def beaker_os_releases(at = nil) | |||
majors = puppet_major_versions | |||
def beaker_os_releases(at = nil, requirement_name = 'openvox') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is this called without a value? I'd make it mandatory. It is breaking the API, but it's a private method anyway. This comes back to my original idea that at
is always the last parameter.
def beaker_os_releases(at = nil, requirement_name = 'openvox') | |
def beaker_os_releases(requirement_name, at = nil) |
puppet_major_versions | ||
when 'openvox' | ||
openvox_major_versions | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end | |
else | |
raise Exception, "Unknown requirement '#{requirement_name}'" | |
end |
beaker_test_matrix(at, 'openvox') + beaker_test_matrix(at, 'puppet') | ||
end | ||
|
||
def beaker_test_matrix(at, requirement_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then also keep at
as the last argument here
def beaker_test_matrix(at, requirement_name) | |
def beaker_test_matrix(requirement_name, at) |
matrix_include = [] | ||
|
||
beaker_os_releases(at) do |os, release, puppet_version| | ||
next if puppet_version_below_minimum?(puppet_version[:value]) | ||
beaker_os_releases(at, requirement_name) do |os, release, puppet_version| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses puppet
variable naming quite often. What is a good way to describe either Puppet or OpenVox? implementation
feels long and generic, but it's the best I can come up with.
# don't raise an exception | ||
# raise Exception, "No #{requirement_name} requirement found" unless requirement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of commented code. I wonder if we should separate out the concerns so you can do the exception handling.
Thinking out loud about APIs:
def puppet_major_versions
requirement = requirements['puppet']
return [] unless requirement
major_version_for_requirement(requirement)
end
def puppet_major_versions!
requirement = requirements['puppet']
raise Exception, 'No Puppet requirement found' unless requirement
major_version_for_requirement(requirement)
end
private
def major_version_for_requirement(requirement)
end
Not that you need puppet_major_versions!
in the current iteration, but it's for an example.
Then taking that a step further, you can generalize it:
def major_versions(requirement_name)
requirement = requirements[requirement_name]
return [] unless requirement
major_version_for_requirement(requirement)
end
def major_versions!(requirement_name)
requirement = requirements[requirement_name]
raise Exception, "No '#{requirement_name}' requirement found' unless requirement
major_version_for_requirement(requirement)
end
private
def major_version_for_requirement(requirement)
end
I like that a lot more because it keeps the PuppetMetadata::Metadata
class very generic without naming individual requirements. We can accept that PuppetMetadata::GithubActions
is a lot more specific while keeping this metadata abstraction as pure as possible.
The one thing that isn't pure is the missing upper bound handling. Technically that's already sort of broken, but in theory it could use some constant that's like this:
REQUIREMENT_UPPER_BOUND_DEFAULTS = {
'openvox' => 8,
'puppet' => 8,
}
But I don't think requirement
has a name so I'd postpone looking at it and keep the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this file BUILDS
refers to what Puppet has AIO builds for. Should that have an extra dimension of requirement_name
?
48ed63f
to
736aa31
Compare
0847570
to
8d7a188
Compare
8d7a188
to
06b2e83
Compare
No description provided.